Select Vue Nodes After Drag #5863

Myestery · 2025-09-30T16:45:53Z

This pull request refactors the node selection logic in the Vue nodes event handler composable to simplify the function signature and improve single vs. multi-selection behavior. The main change is the removal of the wasDragging parameter from the handleNodeSelect function, with selection logic now determined by the current selection state. Related test code is updated to match the new function signature.

Node selection logic improvements:

Refactored the handleNodeSelect function in useNodeEventHandlersIndividual to remove the wasDragging parameter, making the function signature simpler and relying on selection state to handle single vs. multi-selection.
Updated the selection logic to check if multiple nodes are already selected using isLGraphNode, and only perform single selection if not.

Code and test updates:

Updated all calls to handleNodeSelect in the composable to remove the wasDragging argument, ensuring consistent usage throughout the codebase. [1] [2] [3]
Updated all related test cases to use the new handleNodeSelect signature without the third parameter. [1] [2] [3] [4] [5] [6]

Utility import:

Added an import for isLGraphNode from @/utils/litegraphUtil to support the updated selection logic.## Summary

Screenshots (if applicable)

Screen.Recording.2025-09-30.at.17.47.18.mov

… accordingly

github-actions · 2025-09-30T16:46:19Z

🎭 Playwright Test Results

❌ Some tests failed

⏰ Completed at: 10/08/2025, 07:39:36 PM UTC

📈 Summary

Total Tests: 489
Passed: 456 ✅
Failed: 2 ❌
Flaky: 1 ⚠️
Skipped: 30 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 447 / ❌ 2 / ⚠️ 1 / ⏭️ 30
✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

github-actions · 2025-09-30T16:46:21Z

🎨 Storybook Build Status

✅ Build completed successfully!

⏰ Completed at: 10/08/2025, 07:07:08 PM UTC

🔗 Links

📊 View Workflow Run
🎨 View Storybook

🎉 Your Storybook is ready for review!

DrJKL · 2025-09-30T17:45:20Z

src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts

-      // If it wasn't a drag: single-select the node
-      if (!wasDragging) {
+      const selectedMultipleNodes =
+        canvasStore.selectedItems.filter((n) => isLGraphNode(n)).length > 1


This feels a little weird to me. Can we change the selection logic in useNodePointerInteractions instead? I think we'll have to change the interface there anyway since that's where the dragging logic lives.

I would expect the selection to be apparent as I click and drag, not after I release the click.

I agree - I think we should try to match litegraph behavior exactly (unless it's extremely difficult to do).

christian-byrne

It should be fairly comfortable to add a test case here, following the existing patterns:

ComfyUI_frontend/browser_tests/tests/vueNodes/interactions/node/select.spec.ts

Lines 23 to 38 in 35ddb19

    
           test(`should allow selecting multiple nodes with ${name}+click`, async ({ 
        
             comfyPage 
        
           }) => { 
        
             await comfyPage.page.getByText('Load Checkpoint').click() 
        
             expect(await comfyPage.vueNodes.getSelectedNodeCount()).toBe(1) 
        
             await comfyPage.page.getByText('Empty Latent Image').click({ 
        
               modifiers: [modifier] 
        
             }) 
        
             expect(await comfyPage.vueNodes.getSelectedNodeCount()).toBe(2) 
        
             await comfyPage.page.getByText('KSampler').click({ 
        
               modifiers: [modifier] 
        
             }) 
        
             expect(await comfyPage.vueNodes.getSelectedNodeCount()).toBe(3) 
        
           })

src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts

…nter down and update tests accordingly

christian-byrne · 2025-10-03T20:26:22Z

The tests might need to be updated

Myestery · 2025-10-03T21:04:45Z

Screen.Recording.2025-10-03.at.06.43.01.mov

Myestery · 2025-10-03T21:14:55Z

Screen.Recording.2025-10-03.at.06.43.01.mov

Added support for dragging with ctrl and selecting nodes at the same time
Ie node1 is selected, hold ctrl and drag node2
everything drags just like litegraph

…on behavior

christian-byrne

For these tests, we should avoid using litegraph test utils/fixtures (since we are manipulating Vue nodes) and also try to accomplish changes by emulating user behaviors rather than calling methods on code objects. For testing code, we can rely moreso on unit tests; for e2e tests we should test the system using more of a black-box approach - which provides a different breadth of coverage and ensures things work from the most important perspective (user), among other reasons. This means:

Not using NodeReference to get nodes but rather just finding the nodes by using locators that query for visible title text (as a user would do)
Not calling LGraphNode.pin to pin but rather just selecting the node and pressing the pin hotkey (as a user would do).

To see an example of both, check here.

…e real user interaction

src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts

src/renderer/extensions/vueNodes/composables/useNodePointerInteractions.ts

claude

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: Select Vue Nodes After Drag (#5863)
Impact: 189 additions, 121 deletions across 8 files

Issue Distribution

Critical: 0
High: 0
Medium: 2
Low: 2

Category Breakdown

Architecture: 2 issues
Security: 0 issues
Performance: 1 issue
Code Quality: 1 issue

Key Findings

Architecture & Design

This refactoring simplifies the node selection API by removing the wasDragging parameter from handleNodeSelect. However, this introduces two important behavioral changes:

Selection timing change: Selection now occurs on pointer down instead of pointer up, which may affect user experience during drag operations
Selection logic change: The new logic checks for multiple existing selections rather than drag state, which could cause unintended single-selection behavior when clicking on nodes with existing multi-selections

Security Considerations

No security issues identified in this refactoring. The changes maintain the same security boundaries and do not introduce new attack vectors.

Performance Impact

The new hasMultipleNodesSelected function introduces O(n) complexity on every non-multi-select click. While the function includes early termination optimization, it could impact performance with large selection counts.

Integration Points

The changes maintain backward compatibility with existing components and tests. All test files have been properly updated to match the new function signature.

Positive Observations

Well-structured refactoring with clear intent
Comprehensive test coverage updates across all affected files
Good documentation and comments explaining the changes
Proper use of TypeScript types and Vue 3 Composition API patterns
Clean separation of concerns between selection logic and pointer interactions

Next Steps

Verify the behavioral changes are intentional and align with UX expectations
Consider performance optimizations for the selection checking logic
Validate that drag operations work as expected with the new selection timing
Consider grouping import statements for better code organization

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

christian-byrne

LGTM

christian-byrne · 2025-10-08T18:45:58Z

I will fix the conflict

Myestery · 2025-10-08T19:05:45Z

I will fix the conflict

Already fixed locally

Refactor handleNodeSelect to remove unused parameter and update tests…

e1703da

… accordingly

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 30, 2025

Myestery requested a review from christian-byrne September 30, 2025 16:51

DrJKL reviewed Sep 30, 2025

View reviewed changes

christian-byrne reviewed Sep 30, 2025

View reviewed changes

src/renderer/extensions/vueNodes/composables/useNodeEventHandlers.ts Outdated Show resolved Hide resolved

Refactor useNodePointerInteractions to optimize node selection on poi…

fbc47b3

…nter down and update tests accordingly

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Oct 3, 2025

Merge remote-tracking branch 'origin/main' into select-node-after-drag

2243f64

remove test for node-click event on pointer up in LGraphNode.spec.ts

f9c8c26

christian-byrne added the New Browser Test Expectations New browser test screenshot should be set by github action label Oct 5, 2025

Myestery added 2 commits October 7, 2025 01:44

Merge branch 'main' into select-node-after-drag

3323013

feat(NodeReference): add pin and unpin methods with tests for selecti…

7f9c6e4

…on behavior

Myestery added New Browser Test Expectations New browser test screenshot should be set by github action and removed New Browser Test Expectations New browser test screenshot should be set by github action labels Oct 7, 2025

github-actions and others added 2 commits October 7, 2025 01:56

[automated] Update test expectations

043a34b

Merge branch 'main' into select-node-after-drag

6df86b3

Myestery requested a review from christian-byrne October 7, 2025 03:22

christian-byrne requested changes Oct 7, 2025

View reviewed changes

Myestery added 2 commits October 7, 2025 22:11

refactor(NodeReference): remove pin and unpin methods;

da8ee70

refactor(select.spec.ts): streamline pinned node selection test to us…

7dc615b

…e real user interaction

christian-byrne added the claude-review Add to trigger a PR code review from Claude Code label Oct 7, 2025